Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

fix: old VMSS AKS cluster cannot be upgraded after April 2 #1561

Merged
merged 3 commits into from
Jul 11, 2019

Conversation

chengliangli0918
Copy link
Contributor

Reason for Change:
Old VMSS AKS cluster cannot be upgraded after April 2

AKS-Engine changed AKS billing extension type from Compute.AKS-Engine.Linux.Billing to Compute.AKS.Linux.Billing in this PR (bc462f1). The change was vendored into AKS on Apr. 2nd. So any VMSS cluster created before the change will hit this problem during upgrade. New clusters should be fine.

Issue Fixed:
Rename the billing extension to make the ARM deployment happy

Requirements:

Notes:

@acs-bot acs-bot added the size/XS label Jul 2, 2019
@@ -688,6 +688,7 @@ func CreateAgentVMSS(cs *api.ContainerService, profile *api.AgentPoolProfile) Vi
}

if cs.Properties.IsHostedMasterProfile() {
aksBillingExtension.Name = to.StringPtr(fmt.Sprintf("[concat(variables('%sVMNamePrefix'), '-AksLinuxBilling')]", profile.Name))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this going to affect how we track billing data in any way?

Aks should be AKS

What about Windows? This name contains "linux" and will be applied to all extensions, even Windows ones so it might be confusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, before the extension name was computeAksLinuxBilling. This might break how we track these extensions if we query by name computeAksLinuxBilling.

Copy link
Contributor Author

@chengliangli0918 chengliangli0918 Jul 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually before the extension name was set to [vmssname]-computeLinuxBilling, e.g. aks-nodepool1-40022152-vmss-computeAksLinuxBilling. Don't think this name that varies on different VMSS is used to track billing data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. @CecileRobertMichon we'd like to get this merged since this broke existing VMSS clusters. If billing tracking is broken, we take the risk.

@codecov
Copy link

codecov bot commented Jul 2, 2019

Codecov Report

Merging #1561 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1561      +/-   ##
==========================================
+ Coverage   75.86%   75.86%   +<.01%     
==========================================
  Files         128      128              
  Lines       18314    18316       +2     
==========================================
+ Hits        13893    13895       +2     
  Misses       3636     3636              
  Partials      785      785

@CecileRobertMichon
Copy link
Contributor

/hold

verifying the impact of the change on the billing tracking

@CecileRobertMichon
Copy link
Contributor

/hold cancel

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@acs-bot
Copy link

acs-bot commented Jul 11, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, chengliangli0918

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@acs-bot acs-bot merged commit b78fcf1 into Azure:master Jul 11, 2019
CecileRobertMichon pushed a commit to CecileRobertMichon/aks-engine that referenced this pull request Jul 12, 2019
* old VMSS AKS cluster cannot be upgraded after April 2

* use different extension name for AKS Windows/Linux VMSS

* fix unit test
CecileRobertMichon pushed a commit to CecileRobertMichon/aks-engine that referenced this pull request Jul 12, 2019
* old VMSS AKS cluster cannot be upgraded after April 2

* use different extension name for AKS Windows/Linux VMSS

* fix unit test
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants